feat(storage): add signed post policy v4#5576
feat(storage): add signed post policy v4#5576dmitri-lerko wants to merge 2 commits intogoogleapis:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces support for Signed V4 POST policies in the Rust Cloud Storage SDK, aligning it with the capabilities of the Go Storage package. It also establishes a formal roadmap and testing framework to ensure future parity efforts are consistent, well-documented, and rigorously validated against Go reference behaviors. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request establishes a comprehensive plan and initial implementation for bringing the Rust Storage SDK toward parity with the Go Storage library. It introduces a detailed parity report and matrix to track progress across various feature groups, such as ACLs, HMAC keys, and notifications. The initial implementation slice adds support for Signed V4 POST policies, including a new builder, condition types, and extensive conformance testing against existing cross-language fixtures. The feedback provided suggests a minor documentation refinement to point directly to the implementation file in the parity matrix for better clarity.
| | HMAC keys | `storage.HMACKeyHandle` and client methods | Missing | No Rust HMAC helper module | No Rust parity tests yet | | ||
| | Pub/Sub notifications | `storage.Notification` helpers | Missing | No Rust notification helper module | No Rust parity tests yet | | ||
| | Signed V4 URLs | `storage.SignedURL` and bucket signed URLs | Implemented | `src/storage/src/storage/signed_url.rs` | Cross-language conformance coverage exists | | ||
| | Signed V4 POST policies | `storage.GenerateSignedPostPolicyV4` | Implemented | `src/storage/src/storage/post_policy_v4.rs`, `src/storage/src/post_policy_v4.rs` | Go-equivalent conformance, boundary, endpoint style, and Send tests exist | |
There was a problem hiding this comment.
For clarity, you could remove the redundant path to the re-export module from the Evidence column, so it points directly to the implementation file.
| | Signed V4 POST policies | `storage.GenerateSignedPostPolicyV4` | Implemented | `src/storage/src/storage/post_policy_v4.rs`, `src/storage/src/post_policy_v4.rs` | Go-equivalent conformance, boundary, endpoint style, and Send tests exist | | |
| | Signed V4 POST policies | `storage.GenerateSignedPostPolicyV4` | Implemented | `src/storage/src/storage/post_policy_v4.rs` | Go-equivalent conformance, boundary, endpoint style, and Send tests exist | |
|
Thanks for the PR. You will need to sign the CLA or we cannot accept the PR at all. Please split the changes to I think the work for user project duplicates the work in #5490 We would also prefer if you split the work for STORAGE_EMULATOR_HOST to a separate PR from the work for the POST Policy changes. @joshuatants I believe most of the review here will fall on your shoulders. |
405dbd0 to
89e396f
Compare
89e396f to
dc5b7bb
Compare
Hi @coryan , Thank you for your prompt reply. I am happy to take it a step at a time. I'd love to see Go <> Rust SDK parity when it comes to Cloud Storage and Cloud Monitoring. For the latter I am running my own fork in production, but would love to move it over here piecemeal. Please let me know what I can do to make your jobs as easy as possible here. |
|
Hi @dmitri-lerko thank you very much for this. We actually have someone working on the signed post policy v4 and are tracking the issue here (#4746). I should've been more prompt with updating the issue to reflect this. @xlai20 - can you have a look at Dmitri's work? How far along are you with your implementation? How much of Dmitri's work can be used? |
xlai20
left a comment
There was a problem hiding this comment.
Thanks for the PR. It looks moving to the right direction. I've left some comments to request minor changes.
|
|
||
| /// A constraint that the uploaded multipart form must satisfy. | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub enum PostPolicyV4Condition { |
There was a problem hiding this comment.
According to https://docs.cloud.google.com/storage/docs/authentication/signatures#policy-document, there are 3 types of policy conditions: Exact Match, Starts With, and Content Length Range. As here it misses out the Exact Match type, user cannot have the freedom to define exact match condition on fields, whilst such capability is supported in other sdk.
| Self::ContentLengthRange { start, end } | ||
| } | ||
|
|
||
| fn is_empty(&self) -> bool { |
There was a problem hiding this comment.
I think this is_empty() function and its use in filtering out "invalid" policy document conditions is better to be removed in an SDK implementation.
As per the documentation https://docs.cloud.google.com/storage/docs/authentication/signatures#policy-document,
"If a field's value has no restrictions, use the starts-with condition with
["starts-with", "$field", ""],
this suggest that the GCS backend is expecting a starts-with empty string condition. Doing a filtering might end up passing incorrect inputs to the backend.
| impl PostPolicyV4Condition { | ||
| /// Creates a `starts-with` condition. | ||
| /// | ||
| /// Empty values are ignored, matching the Go Storage SDK behavior. |
There was a problem hiding this comment.
Ditto my another comment, don't ignore empty values.
| JsonCondition::Object(BTreeMap::from([(name.to_string(), value.into())])) | ||
| } | ||
|
|
||
| fn escape_like_go_json(json: &str) -> String { |
There was a problem hiding this comment.
Rename this function to "escape_json_str" or "escape_invalid_char".
| escaped | ||
| } | ||
|
|
||
| fn push_json_unicode_escape(output: &mut String, ch: char) { |
There was a problem hiding this comment.
This function is a bit too low-level styled, unlike the usual high-level coding style in Rust, is there a way to improve it?
| fn push_json_unicode_escape(output: &mut String, ch: char) { | ||
| let code = ch as u32; | ||
| if code <= 0xffff { | ||
| output.push_str(&format!("\\u{code:04x}")); |
There was a problem hiding this comment.
As this function is called within the loop of escape function, the use of format! can be a performance hit. Calling the format! macro always allocates a new String on the heap. Try to explore if can change to write! macro to avoid allocations.
Thank you for your guidance and thorough review, I'll aim to iterate on the feedback within a week, |
Summary
PostPolicyV4Builder,PostPolicyV4,PolicyV4Fields, and policy condition helpers fromgoogle-cloud-storagesrc/gax,src/gax-internal, Requester Pays, emulator, or report/plan Markdown changes are includedTesting
cargo test -p google-cloud-storage post_policycargo clippy -p google-cloud-storage --all-targets -- --deny warningsgit diff --checkLatest local focused result: 19 passed, 623 filtered out for
cargo test -p google-cloud-storage post_policy.